Feat: Add ability to duplicate a feature and duplicate as a child#774
Feat: Add ability to duplicate a feature and duplicate as a child#774gsxdsm merged 10 commits intoAutoMaker-Org:v0.15.0rcfrom
Conversation
Removed maintenance warning from README.
Update README to remove maintenance notice
Summary of ChangesHello @gsxdsm, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the application by introducing robust feature duplication capabilities. Users can now create copies of existing features, either as independent entities or as child features that maintain a dependency on their parent. This addresses a previous limitation where duplicate feature names could cause issues during conflict resolution. The backend has been streamlined by removing redundant duplicate title validation, while the frontend provides intuitive access to these new duplication options across various board views. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoved server-side duplicate-title guards on feature create/update; added UI-level feature duplication (sibling/child) across board, kanban card, and list views with new handlers and nested dropdowns; introduced optimistic create/delete cache updates and awaited delete persistence; various lint/config and small server/worktree robustness changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as Board UI
participant Actions as use-board-actions
participant API as Server API
participant DB as Database
participant Events as Event Bus
rect rgba(200,200,255,0.5)
User->>UI: Click "Duplicate" (asChild? true/false)
UI->>Actions: handleDuplicateFeature(feature, asChild)
end
rect rgba(200,255,200,0.5)
Actions->>UI: optimistic cache add (provisional feature)
Actions->>API: POST /features (duplicated payload)
API->>DB: insert feature
DB-->>API: inserted feature (id,status)
API->>Events: emit feature:created
API-->>Actions: return persisted feature
Actions->>UI: reconcile optimistic entry / invalidate cache
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to duplicate features, either as a standalone copy or as a child of the original feature. This is achieved by removing the backend validation that prevented duplicate feature titles and adding the necessary UI elements and logic on the frontend. The core logic for duplication in use-board-actions.ts is well-implemented.
My main feedback is regarding the UI implementation in kanban-card/card-header.tsx and list-view/row-actions.tsx. In both files, the menu for duplicating features has been added in multiple places, leading to significant code duplication. I've left specific comments suggesting how to refactor this by creating a reusable component within each file to improve maintainability.
| {onDuplicate && ( | ||
| <DropdownMenuSub> | ||
| <div className="flex items-center"> | ||
| <DropdownMenuItem | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| onDuplicate(); | ||
| }} | ||
| className="text-xs flex-1 pr-0 rounded-r-none" | ||
| > | ||
| <Copy className="w-3 h-3 mr-2" /> | ||
| Duplicate | ||
| </DropdownMenuItem> | ||
| {onDuplicateAsChild && ( | ||
| <DropdownMenuSubTrigger className="text-xs px-1 rounded-l-none border-l border-border/30 h-8" /> | ||
| )} | ||
| </div> | ||
| {onDuplicateAsChild && ( | ||
| <DropdownMenuSubContent> | ||
| <DropdownMenuItem | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| onDuplicateAsChild(); | ||
| }} | ||
| className="text-xs" | ||
| > | ||
| <GitFork className="w-3 h-3 mr-2" /> | ||
| Duplicate as Child | ||
| </DropdownMenuItem> | ||
| </DropdownMenuSubContent> | ||
| )} | ||
| </DropdownMenuSub> | ||
| )} |
There was a problem hiding this comment.
There's significant code duplication for the 'Duplicate' feature menu. This same block of JSX is repeated four times in this file for different feature statuses (running, backlog, waiting/verified, and in-progress).
To improve maintainability and reduce redundancy, I suggest extracting this logic into a dedicated component within this file.
Here's an example of what that component could look like:
const DuplicateFeatureSubMenu = ({
onDuplicate,
onDuplicateAsChild,
}: {
onDuplicate?: () => void;
onDuplicateAsChild?: () => void;
}) => {
if (!onDuplicate) {
return null;
}
return (
<DropdownMenuSub>
<div className="flex items-center">
<DropdownMenuItem
onClick={(e) => {
e.stopPropagation();
onDuplicate();
}}
className="text-xs flex-1 pr-0 rounded-r-none"
>
<Copy className="w-3 h-3 mr-2" />
Duplicate
</DropdownMenuItem>
{onDuplicateAsChild && (
<DropdownMenuSubTrigger className="text-xs px-1 rounded-l-none border-l border-border/30 h-8" />
)}
</div>
{onDuplicateAsChild && (
<DropdownMenuSubContent>
<DropdownMenuItem
onClick={(e) => {
e.stopPropagation();
onDuplicateAsChild();
}}
className="text-xs"
>
<GitFork className="w-3 h-3 mr-2" />
Duplicate as Child
</DropdownMenuItem>
</DropdownMenuSubContent>
)}
</DropdownMenuSub>
);
};You could then replace each duplicated block with:
<DuplicateFeatureSubMenu onDuplicate={onDuplicate} onDuplicateAsChild={onDuplicateAsChild} />| {handlers.onDuplicate && ( | ||
| <DropdownMenuSub> | ||
| <div className="flex items-center"> | ||
| <DropdownMenuItem | ||
| onClick={withClose(handlers.onDuplicate)} | ||
| className="flex-1 pr-0 rounded-r-none" | ||
| > | ||
| <Copy className="w-4 h-4 mr-2" /> | ||
| Duplicate | ||
| </DropdownMenuItem> | ||
| {handlers.onDuplicateAsChild && ( | ||
| <DropdownMenuSubTrigger className="px-1 rounded-l-none border-l border-border/30 h-8" /> | ||
| )} | ||
| </div> | ||
| {handlers.onDuplicateAsChild && ( | ||
| <DropdownMenuSubContent> | ||
| <MenuItem | ||
| icon={GitFork} | ||
| label="Duplicate as Child" | ||
| onClick={withClose(handlers.onDuplicateAsChild)} | ||
| /> | ||
| </DropdownMenuSubContent> | ||
| )} | ||
| </DropdownMenuSub> | ||
| )} |
There was a problem hiding this comment.
Similar to the changes in kanban-card/card-header.tsx, the JSX for the 'Duplicate' feature menu is duplicated multiple times in this file (five times, to be exact). This makes the component harder to read and maintain.
I recommend refactoring this into a reusable component within this file. This will make the code cleaner and easier to manage if the duplicate menu ever needs to be changed.
Here's a possible implementation for the component:
const DuplicateFeatureSubMenu = ({
handlers,
withClose,
}: {
handlers: { onDuplicate?: () => void; onDuplicateAsChild?: () => void };
withClose: (handler: () => void) => () => void;
}) => {
if (!handlers.onDuplicate) {
return null;
}
return (
<DropdownMenuSub>
<div className="flex items-center">
<DropdownMenuItem
onClick={withClose(handlers.onDuplicate)}
className="flex-1 pr-0 rounded-r-none"
>
<Copy className="w-4 h-4 mr-2" />
Duplicate
</DropdownMenuItem>
{handlers.onDuplicateAsChild && (
<DropdownMenuSubTrigger className="px-1 rounded-l-none border-l border-border/30 h-8" />
)}
</div>
{handlers.onDuplicateAsChild && (
<DropdownMenuSubContent>
<MenuItem
icon={GitFork}
label="Duplicate as Child"
onClick={withClose(handlers.onDuplicateAsChild)}
/>
</DropdownMenuSubContent>
)}
</DropdownMenuSub>
);
};You can then use it like this in place of the repeated blocks:
<DuplicateFeatureSubMenu handlers={handlers} withClose={withClose} />![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/ui/src/components/views/board-view/hooks/use-board-actions.ts (1)
1086-1104: Consider stripping run‑state fields when duplicating a feature.Spreading the full
Featurecopies transient state (e.g.,planSpec,startedAt,error,summary,passes), which can make the duplicate look pre-run. If that’s not intended, omit those fields before callinghandleAddFeature.♻️ Suggested refinement
- const { id: _id, status: _status, ...featureData } = feature; + const { + id: _id, + status: _status, + startedAt: _startedAt, + justFinishedAt: _justFinishedAt, + planSpec: _planSpec, + error: _error, + summary: _summary, + passes: _passes, + titleGenerating: _titleGenerating, + descriptionHistory: _descriptionHistory, + ...featureData + } = feature;Also applies to: 1126-1126
apps/ui/src/components/views/board-view/components/list-view/row-actions.tsx (1)
414-437: Optional: extract the duplicate submenu to reduce repetition.The same
DropdownMenuSubblock is repeated across status branches; a small helper would cut maintenance overhead.Also applies to: 491-515, 562-586, 638-662, 690-714
Add optimistic UI updates with rollback capability for feature creation and deletion operations. Await persistFeatureDelete promise and add Playwright testing dependency.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ui/src/components/views/board-view/hooks/use-board-persistence.ts (1)
119-148:⚠️ Potential issue | 🟠 MajorEarly return on missing API skips rollback of optimistic deletion.
If
api.featuresis falsy (line 130–132), the function returns after the optimistic removal on lines 123–126 without restoringpreviousFeaturesor invalidating the cache. The feature silently disappears from the board until the next refetch.Proposed fix
const api = getElectronAPI(); if (!api.features) { logger.error('Features API not available'); + // Rollback optimistic update + if (previousFeatures) { + queryClient.setQueryData(queryKeys.features.all(currentProject.path), previousFeatures); + } return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/hooks/use-board-persistence.ts` around lines 119 - 148, The early return when api.features is falsy causes the optimistic deletion to never be rolled back; modify the block around getElectronAPI()/api.features in useBoardPersistence hook so that if api.features is missing you restore the cached data (use queryClient.setQueryData with previousFeatures when present) and call queryClient.invalidateQueries for queryKeys.features.all(currentProject.path) before returning, and keep logging via logger.error; also ensure you still return afterward to abort the network call path but only after rollback and invalidation are performed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ui/src/components/views/board-view/hooks/use-board-actions.ts`:
- Around line 1086-1104: handleDuplicateFeature is currently only stripping id
and status, causing runtime/state fields to be copied into the duplicate; update
the destructuring in handleDuplicateFeature to also remove known runtime fields
(e.g., startedAt, error, summary, spec, passes, planSpec, descriptionHistory,
titleGenerating) so they are not included in duplicatedFeatureData before
calling handleAddFeature; ensure you still preserve the original behavior for
dependencies when asChild is true and keep the toast notification unchanged.
---
Outside diff comments:
In `@apps/ui/src/components/views/board-view/hooks/use-board-persistence.ts`:
- Around line 119-148: The early return when api.features is falsy causes the
optimistic deletion to never be rolled back; modify the block around
getElectronAPI()/api.features in useBoardPersistence hook so that if
api.features is missing you restore the cached data (use
queryClient.setQueryData with previousFeatures when present) and call
queryClient.invalidateQueries for queryKeys.features.all(currentProject.path)
before returning, and keep logging via logger.error; also ensure you still
return afterward to abort the network call path but only after rollback and
invalidation are performed.
…-festure # Conflicts: # apps/ui/src/components/views/board-view/components/kanban-card/card-header.tsx # apps/ui/src/components/views/board-view/components/kanban-card/kanban-card.tsx # apps/ui/src/components/views/board-view/hooks/use-board-persistence.ts
There was a problem hiding this comment.
🧹 Nitpick comments (5)
apps/ui/src/components/views/board-view/hooks/use-board-persistence.ts (3)
88-114: Optimistic rollback on create failure relies solely oninvalidateQueries, which is async and not awaited.In the error path (lines 105-111), the optimistic entry added at line 91 is only removed via an un-awaited
invalidateQueries. Until the background refetch completes, the phantom feature remains visible in the UI. Compare this withpersistFeatureDelete(lines 153-154), which synchronously restores the previous cache snapshot before invalidating.Consider saving the previous cache state (like you do for delete) and restoring it synchronously on error:
Proposed fix
// Optimistically add to React Query cache for immediate board refresh + const previousFeatures = queryClient.getQueryData<Feature[]>( + queryKeys.features.all(currentProject.path) + ); queryClient.setQueryData<Feature[]>( queryKeys.features.all(currentProject.path), (existing) => (existing ? [...existing, feature] : [feature]) ); try { // ... success path unchanged ... } catch (error) { logger.error('Failed to persist feature creation:', error); - // Rollback optimistic update on error + // Rollback optimistic update synchronously, then invalidate + if (previousFeatures) { + queryClient.setQueryData(queryKeys.features.all(currentProject.path), previousFeatures); + } queryClient.invalidateQueries({ queryKey: queryKeys.features.all(currentProject.path), }); throw error; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/hooks/use-board-persistence.ts` around lines 88 - 114, The optimistic create currently pushes the new feature via queryClient.setQueryData but on error only calls queryClient.invalidateQueries (async) so the phantom item remains; capture the previous cache snapshot before calling queryClient.setQueryData (use the same queryKey used: queryKeys.features.all(currentProject.path)), then in the catch restore that snapshot synchronously via queryClient.setQueryData (reverting the optimistic add) before calling queryClient.invalidateQueries; update the persistFeatureCreate flow (where setQueryData and updateFeature are used) to mirror the synchronous rollback pattern used in persistFeatureDelete.
131-143: Inconsistent error-handling contract: delete silently returns on missing API, create throws.When
api.featuresis unavailable,persistFeatureCreate(line 85) throws anErrorso callers can react, whilepersistFeatureDelete(line 134) logs and silently returns. The rollback here is good, but callers of delete have no way to know the operation was a no-op. If this asymmetry is intentional (the comment on line 78 suggests create's throw is deliberate), it's worth a brief inline comment explaining why delete swallows the error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/hooks/use-board-persistence.ts` around lines 131 - 143, The delete path in use-board-persistence (persistFeatureDelete) silently logs and returns when api.features is missing, while persistFeatureCreate throws; make the contract consistent by either rethrowing an Error after performing the optimistic-rollback (so callers can detect the no-op) or explicitly document the asymmetry with a short inline comment; locate persistFeatureDelete and modify the block that checks if (!api.features) to perform the rollback via queryClient.setQueryData/queryClient.invalidateQueries and then throw a descriptive Error (e.g., "Features API not available") so callers receive the same failure signal as persistFeatureCreate, or add a comment explaining why delete intentionally swallows the error if you choose not to throw.
54-66:setQueryDataimmediately followed byinvalidateQuerieson the same key — minor redundancy.On the success path of
persistFeatureUpdate, the cache is updated synchronously (lines 54-62) and then immediately invalidated (lines 64-66), which triggers a refetch that overwrites the just-set data. ThesetQueryDataprovides an instant UI update while the refetch is in-flight, so this is a valid optimistic pattern — just noting that the same approach is not used inpersistFeatureCreate's success path (where onlyupdateFeature+ invalidation are used, with nosetQueryData). If you'd like consistency across all three operations, consider applying the same set-then-invalidate pattern to create's success path as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/hooks/use-board-persistence.ts` around lines 54 - 66, The update path currently calls queryClient.setQueryData(...) for queryKeys.features.all(currentProject.path) and then immediately queryClient.invalidateQueries(...) which is a minor redundancy but provides optimistic UI; make the create path (persistFeatureCreate) consistent by adding the same setQueryData(...) update for the newly created Feature before its invalidateQueries(...) call (use the same queryKey: queryKeys.features.all(currentProject.path>) and same mapping logic as in the persistFeatureUpdate success handler) so both create and update follow the set-then-invalidate pattern.apps/ui/src/components/views/board-view.tsx (1)
1507-1508: InconsistentonDuplicateprop signatures between ListView and KanbanBoard.For
ListView,onDuplicateandonDuplicateAsChildare separate callbacks each receiving aFeature. ForKanbanBoard,onDuplicateis a single callback(feature, asChild) => voidpassed directly ashandleDuplicateFeature. This works but the asymmetry means the two views have different API contracts for the same logical action. Not blocking, but worth noting for consistency if these interfaces are refactored later.Also applies to: 1548-1548
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view.tsx` around lines 1507 - 1508, The KanbanBoard prop usage is asymmetric with ListView: instead of passing a single onDuplicate (feature, asChild) handler, pass two separate callbacks to KanbanBoard so both views share the same API: replace the single onDuplicate={handleDuplicateFeature} with onDuplicate={(feature) => handleDuplicateFeature(feature, false)} and onDuplicateAsChild={(feature) => handleDuplicateFeature(feature, true)} (use the existing handleDuplicateFeature function) so ListView and KanbanBoard have consistent onDuplicate / onDuplicateAsChild signatures.apps/ui/src/components/views/board-view/components/kanban-card/card-header.tsx (1)
125-157: Extract the duplicated dropdown menu block into a shared component.The same ~30-line duplicate/submenu JSX block is copy-pasted verbatim four times (running, backlog, waiting/verified, in_progress sections). This violates DRY and makes future changes error-prone.
♻️ Suggested extraction
Create a small local helper, e.g.:
function DuplicateMenuItems({ onDuplicate, onDuplicateAsChild, }: { onDuplicate?: () => void; onDuplicateAsChild?: () => void; }) { if (!onDuplicate) return null; return ( <DropdownMenuSub> <div className="flex items-center"> <DropdownMenuItem onClick={(e) => { e.stopPropagation(); onDuplicate(); }} className="text-xs flex-1 pr-0 rounded-r-none" > <Copy className="w-3 h-3 mr-2" /> Duplicate </DropdownMenuItem> {onDuplicateAsChild && ( <DropdownMenuSubTrigger className="text-xs px-1 rounded-l-none border-l border-border/30 h-8" /> )} </div> {onDuplicateAsChild && ( <DropdownMenuSubContent> <DropdownMenuItem onClick={(e) => { e.stopPropagation(); onDuplicateAsChild(); }} className="text-xs" > <GitFork className="w-3 h-3 mr-2" /> Duplicate as Child </DropdownMenuItem> </DropdownMenuSubContent> )} </DropdownMenuSub> ); }Then replace each of the four blocks with:
<DuplicateMenuItems onDuplicate={onDuplicate} onDuplicateAsChild={onDuplicateAsChild} />Also applies to: 220-252, 340-372, 443-475
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/components/kanban-card/card-header.tsx` around lines 125 - 157, Extract the repeated ~30-line JSX dropdown into a small reusable component (e.g., DuplicateMenuItems) and replace each copy with a single invocation; implement DuplicateMenuItems to accept props onDuplicate?: () => void and onDuplicateAsChild?: () => void, return null if onDuplicate is falsy, and render the same structure currently used (using DropdownMenuSub, DropdownMenuItem, DropdownMenuSubTrigger, DropdownMenuSubContent, Copy, GitFork) while preserving the stopPropagation handlers and className values; then update the four occurrences in card-header.tsx to <DuplicateMenuItems onDuplicate={onDuplicate} onDuplicateAsChild={onDuplicateAsChild} /> to remove duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/ui/src/components/views/board-view.tsx`:
- Around line 1507-1508: The KanbanBoard prop usage is asymmetric with ListView:
instead of passing a single onDuplicate (feature, asChild) handler, pass two
separate callbacks to KanbanBoard so both views share the same API: replace the
single onDuplicate={handleDuplicateFeature} with onDuplicate={(feature) =>
handleDuplicateFeature(feature, false)} and onDuplicateAsChild={(feature) =>
handleDuplicateFeature(feature, true)} (use the existing handleDuplicateFeature
function) so ListView and KanbanBoard have consistent onDuplicate /
onDuplicateAsChild signatures.
In
`@apps/ui/src/components/views/board-view/components/kanban-card/card-header.tsx`:
- Around line 125-157: Extract the repeated ~30-line JSX dropdown into a small
reusable component (e.g., DuplicateMenuItems) and replace each copy with a
single invocation; implement DuplicateMenuItems to accept props onDuplicate?: ()
=> void and onDuplicateAsChild?: () => void, return null if onDuplicate is
falsy, and render the same structure currently used (using DropdownMenuSub,
DropdownMenuItem, DropdownMenuSubTrigger, DropdownMenuSubContent, Copy, GitFork)
while preserving the stopPropagation handlers and className values; then update
the four occurrences in card-header.tsx to <DuplicateMenuItems
onDuplicate={onDuplicate} onDuplicateAsChild={onDuplicateAsChild} /> to remove
duplication.
In `@apps/ui/src/components/views/board-view/hooks/use-board-persistence.ts`:
- Around line 88-114: The optimistic create currently pushes the new feature via
queryClient.setQueryData but on error only calls queryClient.invalidateQueries
(async) so the phantom item remains; capture the previous cache snapshot before
calling queryClient.setQueryData (use the same queryKey used:
queryKeys.features.all(currentProject.path)), then in the catch restore that
snapshot synchronously via queryClient.setQueryData (reverting the optimistic
add) before calling queryClient.invalidateQueries; update the
persistFeatureCreate flow (where setQueryData and updateFeature are used) to
mirror the synchronous rollback pattern used in persistFeatureDelete.
- Around line 131-143: The delete path in use-board-persistence
(persistFeatureDelete) silently logs and returns when api.features is missing,
while persistFeatureCreate throws; make the contract consistent by either
rethrowing an Error after performing the optimistic-rollback (so callers can
detect the no-op) or explicitly document the asymmetry with a short inline
comment; locate persistFeatureDelete and modify the block that checks if
(!api.features) to perform the rollback via
queryClient.setQueryData/queryClient.invalidateQueries and then throw a
descriptive Error (e.g., "Features API not available") so callers receive the
same failure signal as persistFeatureCreate, or add a comment explaining why
delete intentionally swallows the error if you choose not to throw.
- Around line 54-66: The update path currently calls
queryClient.setQueryData(...) for queryKeys.features.all(currentProject.path)
and then immediately queryClient.invalidateQueries(...) which is a minor
redundancy but provides optimistic UI; make the create path
(persistFeatureCreate) consistent by adding the same setQueryData(...) update
for the newly created Feature before its invalidateQueries(...) call (use the
same queryKey: queryKeys.features.all(currentProject.path>) and same mapping
logic as in the persistFeatureUpdate success handler) so both create and update
follow the set-then-invalidate pattern.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/server/src/routes/worktree/routes/checkout-branch.ts (1)
57-57:⚠️ Potential issue | 🟠 Major
worktreePathis also unsanitized user input passed tocwdand shell commands.
worktreePathcomes directly fromreq.bodywith only a truthiness check. A crafted path could traverse directories or, combined with shell execution, cause unintended side effects. Consider validating it against an allowlist of known worktree directories or at minimum resolving and confirming it lives under the expected workspace root.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/worktree/routes/checkout-branch.ts` at line 57, worktreePath from req.body is used unsanitized in execAsync and as cwd (symbols: worktreePath, execAsync, branchName), so validate/resolve it before use: resolve the input with path.resolve and ensure the resolved path is inside the expected workspace root (compare prefixes) or check against an allowlist of permitted worktree directories; reject inputs containing traversal or outside-root results. After resolving and validating, assert the path exists and is a directory (fs.stat) and optionally verify it contains a Git worktree (e.g., .git or git rev-parse sanity check run from the validated cwd), then pass only the validated resolved path as cwd to execAsync; do not call shell commands with raw user input.apps/ui/src/components/views/board-view/components/kanban-card/card-header.tsx (1)
1-1: 🛠️ Refactor suggestion | 🟠 Major
@ts-nocheckdisables all type safety for this file.This suppresses every TypeScript diagnostic, hiding real type errors across all ~490 lines. The comment justification ("header component props with optional handlers and status variants") doesn't indicate an actual type issue. If there's a specific problematic line, prefer a targeted
@ts-expect-errorwith a reason. Otherwise, removing this directive will let the compiler catch regressions in the new prop plumbing (onDuplicate,onDuplicateAsChild).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/components/kanban-card/card-header.tsx` at line 1, Remove the top-level "@ts-nocheck" in card-header.tsx and restore proper typing for the header component's props (including onDuplicate and onDuplicateAsChild) so TypeScript can validate the ~490-line file; if there's one specific problematic expression, replace the blanket suppression with targeted "@ts-expect-error <reason>" immediately above that expression instead of disabling all checks. Locate the component/props interface (the header component definition and its props object) and declare accurate types for handlers and status variants (or import the existing prop type/interface), run the typechecker, and fix any resulting type errors rather than masking them. Ensure handlers like onDuplicate and onDuplicateAsChild are typed as optional functions (e.g., (() => void) | undefined) so callers and internal usage are type-safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/routes/worktree/routes/checkout-branch.ts`:
- Line 40: The current invalidChars regex in checkout-branch.ts is incorrect
(the `[` escape was dropped) and is not a sufficient defense against shell
injection for the branchName used in exec calls; restore the escaped bracket in
the regex (use `/[\s~^:?*\[\\]/` or better, replace with a conservative
allowlist such as `/^[A-Za-z0-9._\-\/]+$/`) and remove shell interpolation by
switching from child_process.exec to execFile/spawn with argument arrays (or
pass branchName as an explicit argv item) where branchName is used in the file's
exec calls so the branch name is never interpreted by a shell.
In
`@apps/ui/src/components/views/board-view/components/kanban-card/card-header.tsx`:
- Around line 220-239: The overflow dropdown currently renders a completely
empty menu when DuplicateMenuItems returns null (e.g., when onDuplicate is
undefined); update the card header to avoid showing an empty menu by
conditionally rendering the DropdownMenu/DropdownMenuTrigger (the MoreVertical
Button) only when there are actionable menu items (check onDuplicate and
onDuplicateAsChild) or alternatively move at least one always-available action
(e.g., Edit) into DuplicateMenuItems so it never returns null; locate the JSX
around DropdownMenu / DropdownMenuTrigger and DuplicateMenuItems in
card-header.tsx and wrap the DropdownMenu (or the Trigger Button) with a guard
like "if (onDuplicate || onDuplicateAsChild || hasOtherActions)" to prevent
opening an empty menu.
- Around line 34-75: The DuplicateMenuItems component currently renders a
DropdownMenuSub even when onDuplicateAsChild is falsy and places a
DropdownMenuItem inside a raw div, which breaks Radix's expected structure and
keyboard/accessibility behavior; fix this by splitting into two explicit
branches inside DuplicateMenuItems: if onDuplicateAsChild is falsy return a
single DropdownMenuItem that calls onDuplicate; if onDuplicateAsChild is truthy
render a proper DropdownMenuSub composed only of a DropdownMenuSubTrigger (for
the chevron/expand control) and a separate clickable DropdownMenuItem for the
primary "Duplicate" action (ensuring the primary item is not wrapped in a raw
div but placed as a sibling or properly nested per Radix docs), and include
DropdownMenuSubContent with the "Duplicate as Child" DropdownMenuItem that calls
onDuplicateAsChild — reference DuplicateMenuItems, DropdownMenuSub,
DropdownMenuItem, DropdownMenuSubTrigger, and DropdownMenuSubContent when making
the changes.
---
Outside diff comments:
In `@apps/server/src/routes/worktree/routes/checkout-branch.ts`:
- Line 57: worktreePath from req.body is used unsanitized in execAsync and as
cwd (symbols: worktreePath, execAsync, branchName), so validate/resolve it
before use: resolve the input with path.resolve and ensure the resolved path is
inside the expected workspace root (compare prefixes) or check against an
allowlist of permitted worktree directories; reject inputs containing traversal
or outside-root results. After resolving and validating, assert the path exists
and is a directory (fs.stat) and optionally verify it contains a Git worktree
(e.g., .git or git rev-parse sanity check run from the validated cwd), then pass
only the validated resolved path as cwd to execAsync; do not call shell commands
with raw user input.
In
`@apps/ui/src/components/views/board-view/components/kanban-card/card-header.tsx`:
- Line 1: Remove the top-level "@ts-nocheck" in card-header.tsx and restore
proper typing for the header component's props (including onDuplicate and
onDuplicateAsChild) so TypeScript can validate the ~490-line file; if there's
one specific problematic expression, replace the blanket suppression with
targeted "@ts-expect-error <reason>" immediately above that expression instead
of disabling all checks. Locate the component/props interface (the header
component definition and its props object) and declare accurate types for
handlers and status variants (or import the existing prop type/interface), run
the typechecker, and fix any resulting type errors rather than masking them.
Ensure handlers like onDuplicate and onDuplicateAsChild are typed as optional
functions (e.g., (() => void) | undefined) so callers and internal usage are
type-safe.
apps/ui/src/components/views/board-view/components/kanban-card/card-header.tsx
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/components/kanban-card/card-header.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/ui/src/components/views/board-view/components/kanban-card/card-header.tsx (1)
315-349: Consider guarding overflow menu visibility here too.Unlike the backlog section (line 237), this overflow menu for waiting/verified status is always rendered. It won't be empty (it always has "Spawn Sub-Task"), but if
onSpawnTaskis also undefined, that item becomes a no-op click. For consistency with the backlog pattern, you could guard this menu ononSpawnTask || onDuplicate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/components/kanban-card/card-header.tsx` around lines 315 - 349, The overflow menu is always rendered even when its actions are no-ops; guard its rendering by wrapping the DropdownMenu block (the DropdownMenu, DropdownMenuTrigger/Button, DropdownMenuContent, DropdownMenuItem, and DuplicateMenuItems components) in a conditional that checks for actionable callbacks (e.g., onSpawnTask || onDuplicate) so the menu is only added when at least one action is provided; keep references to feature.status/feature.id for the data-testid values and still pass onDuplicateAsChild through to DuplicateMenuItems.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/ui/src/components/views/board-view/components/kanban-card/card-header.tsx`:
- Around line 209-210: The code is silencing a TypeScript error instead of
fixing the type: add the missing literal 'ready' to the
FeatureStatusWithPipeline union in the type definitions so TS knows it's a valid
runtime status; update the union in libs/types/src/pipeline.ts (where
FeatureStatusWithPipeline is declared) to include 'ready' and run type checks,
and verify usages in feature-state-manager.ts, auto-mode filters, and components
like card-header.tsx now type-check without the `@ts-expect-error`.
---
Duplicate comments:
In
`@apps/ui/src/components/views/board-view/components/kanban-card/card-header.tsx`:
- Around line 33-90: DuplicateMenuItems should keep the two-branch approach: if
onDuplicate is falsy, return null; if onDuplicateAsChild is falsy, render a
single DropdownMenuItem that calls onDuplicate; otherwise render a
DropdownMenuSub containing DropdownMenuSubTrigger and DropdownMenuSubContent
with two DropdownMenuItem entries that call onDuplicate and onDuplicateAsChild
respectively (each stopping event propagation). Locate and update the
DuplicateMenuItems function and ensure the components DropdownMenuItem,
DropdownMenuSub, DropdownMenuSubTrigger, DropdownMenuSubContent use the existing
onDuplicate and onDuplicateAsChild callbacks and e.stopPropagation() as shown.
- Around line 236-258: The overflow menu rendering is correctly guarded to avoid
an empty DropdownMenu; keep the conditional check using onDuplicate to only
render <DropdownMenu> and its children (DropdownMenuTrigger, Button,
DropdownMenuContent, DuplicateMenuItems) and ensure the props onDuplicate and
onDuplicateAsChild remain passed through unchanged so the menu only appears when
actionable items exist.
---
Nitpick comments:
In
`@apps/ui/src/components/views/board-view/components/kanban-card/card-header.tsx`:
- Around line 315-349: The overflow menu is always rendered even when its
actions are no-ops; guard its rendering by wrapping the DropdownMenu block (the
DropdownMenu, DropdownMenuTrigger/Button, DropdownMenuContent, DropdownMenuItem,
and DuplicateMenuItems components) in a conditional that checks for actionable
callbacks (e.g., onSpawnTask || onDuplicate) so the menu is only added when at
least one action is provided; keep references to feature.status/feature.id for
the data-testid values and still pass onDuplicateAsChild through to
DuplicateMenuItems.
apps/ui/src/components/views/board-view/components/kanban-card/card-header.tsx
Outdated
Show resolved
Hide resolved
2 participants
Add ability to duplicate a feature. Fixes an issue where pull and resolve conflicts fails the second time because a duplicate feature name exists.
This feature is useful for running a feature through multiple times, trying different models or to ensure correctness. A duplicate can be created as a child such that the feature can be run in a loop in auto mode (Ralph style)
Summary by CodeRabbit
New Features
Behavior / UX Changes
Refactor
Style / Chores
Documentation